fix(ui): exclude display name from required-field validation in tool form#3464
Conversation
There was a problem hiding this comment.
Code Review Summary for PR #3464
Issue: #3433 - Unable to create a tool without a display name
Changes: Modified admin.js:16625-16627 to fix form validation selector
✅ Fix Validation
The PR correctly addresses the reported bug:
- Root Cause: The wildcard selector
input[name*="name"]was matching ALL fields containing "name", including the optionaldisplayNamefield - Solution: Changed to exact attribute matching:
input[name="name"], input[name="customName"], input[name="custom_name"] - Result: Optional
displayNamefields are no longer incorrectly validated as required
📊 Review Findings
1 issues identified:
- Medium Severity: Missing test coverage for the validation exclusion fix
✅ Strengths
- Minimal, surgical fix that directly addresses the bug
- Preserves validation for actual required name fields
- Handles multiple naming conventions (
name,customName,custom_name) - Clear inline comment explaining the change
🔍 Key Recommendations
- Add automated tests to prevent regression (most important)
|
Thanks @Nayana-R-Gowda — correct fix for #3433. Narrowing the querySelector to only target technical name fields is the right approach. LGTM. |
|
The validation selector was updated to target only the actual technical name fields (name, customName, custom_name) so that optional displayName fields are no longer validated as required. |
…form The name-field validation selector used a wildcard (input[name*="name"]) that also matched displayName and display_name inputs, making the optional display-name field effectively required. Switch to explicit selectors for technical name fields only, and filter out hidden inputs so edit-form hidden name fields are also skipped. Closes #3433 Signed-off-by: NAYANA.R <nayana.r7813@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Cover the name-field selection logic introduced in the previous commit: - visible name and customName inputs are validated on blur - displayName and display_name inputs are excluded - hidden name inputs (edit forms) are excluded - error styling is applied/cleared correctly - combined form scenarios (create + edit layouts) Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
47dd363 to
92f9c9a
Compare
crivetimihai
left a comment
There was a problem hiding this comment.
Reviewed and verified — the fix is correct and well-targeted.
Code review notes:
- The old wildcard selector
input[name*="name"]matcheddisplayNameanddisplay_namefields, making the optional display name effectively required. The new explicit selectors (input[name="name"], input[name="customName"], input[name="custom_name"]) correctly target only technical name fields. - The hidden-input filter correctly excludes the
name="name"hidden field in edit forms. - No security concerns — backend
validate_display_name()validators inschemas.pystill sanitize display names server-side. - No performance concerns.
Changes made during review:
- Rebased onto
mainand squashed 5 commits (2 feature + 3 merge) into 1 clean commit, preserving original author. - Added 9 unit tests in
tests/js/admin-form-validation.test.jscovering all branches: visible name validated, customName validated, displayName excluded, display_name excluded, hidden name excluded, error styling applied/cleared, and combined create/edit form scenarios. - Verified E2E via Playwright: tool creation succeeds with empty display name, tool appears in the table.
All 831 JS tests pass.
Maintainer review changesRebased onto Commits after rebase
Tests added (
|
| Test | Validates |
|---|---|
visible input[name="name"] |
validated on blur |
input[name="customName"] |
validated on blur |
input[name="displayName"] |
excluded from validation |
input[name="display_name"] |
excluded from validation |
hidden input[name="name"] |
excluded from validation |
| valid name clears styling | error classes removed |
| invalid name shows styling | border-red-500 applied |
| combined name + displayName form | only name validated |
| edit form layout | hidden name excluded, customName validated, displayName excluded |
E2E verification
Verified via Playwright: created a tool with Display Name left empty — form submitted successfully, tool appeared in the table.
…form (IBM#3464) * fix(ui): exclude display name from required-field validation in tool form The name-field validation selector used a wildcard (input[name*="name"]) that also matched displayName and display_name inputs, making the optional display-name field effectively required. Switch to explicit selectors for technical name fields only, and filter out hidden inputs so edit-form hidden name fields are also skipped. Closes IBM#3433 Signed-off-by: NAYANA.R <nayana.r7813@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * test(ui): add setupFormValidation tests for display name exclusion Cover the name-field selection logic introduced in the previous commit: - visible name and customName inputs are validated on blur - displayName and display_name inputs are excluded - hidden name inputs (edit forms) are excluded - error styling is applied/cleared correctly - combined form scenarios (create + edit layouts) Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: NAYANA.R <nayana.r7813@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Dumitru C <canteadumitru@gmail.com>
…form (#3464) * fix(ui): exclude display name from required-field validation in tool form The name-field validation selector used a wildcard (input[name*="name"]) that also matched displayName and display_name inputs, making the optional display-name field effectively required. Switch to explicit selectors for technical name fields only, and filter out hidden inputs so edit-form hidden name fields are also skipped. Closes #3433 Signed-off-by: NAYANA.R <nayana.r7813@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * test(ui): add setupFormValidation tests for display name exclusion Cover the name-field selection logic introduced in the previous commit: - visible name and customName inputs are validated on blur - displayName and display_name inputs are excluded - hidden name inputs (edit forms) are excluded - error styling is applied/cleared correctly - combined form scenarios (create + edit layouts) Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: NAYANA.R <nayana.r7813@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
…form (#3464) * fix(ui): exclude display name from required-field validation in tool form The name-field validation selector used a wildcard (input[name*="name"]) that also matched displayName and display_name inputs, making the optional display-name field effectively required. Switch to explicit selectors for technical name fields only, and filter out hidden inputs so edit-form hidden name fields are also skipped. Closes #3433 Signed-off-by: NAYANA.R <nayana.r7813@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * test(ui): add setupFormValidation tests for display name exclusion Cover the name-field selection logic introduced in the previous commit: - visible name and customName inputs are validated on blur - displayName and display_name inputs are excluded - hidden name inputs (edit forms) are excluded - error styling is applied/cleared correctly - combined form scenarios (create + edit layouts) Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: NAYANA.R <nayana.r7813@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Yosief Eyob <yosiefogbazion@gmail.com>
Signed-off-by: NAYANA.R nayana.r7813@gmail.com
closes #3433
💡 Fix Description
ixed validation selector in admin.js so displayName is no longer treated like the required technical name field.
🧪 Verification
make lintmake test📐 MCP Compliance (if relevant)
✅ Checklist
make black isort pre-commit)